-
Notifications
You must be signed in to change notification settings - Fork 224
Greenbids: Populate AppliedTo and change structure of analytics results in Analytics Tag #3728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...ks/modules/greenbids/real/time/data/v1/GreenbidsRealTimeDataProcessedAuctionRequestHook.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
...ks/modules/greenbids/real/time/data/v1/GreenbidsRealTimeDataProcessedAuctionRequestHook.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Show resolved
Hide resolved
...ks/modules/greenbids/real/time/data/v1/GreenbidsRealTimeDataProcessedAuctionRequestHook.java
Outdated
Show resolved
Hide resolved
...ks/modules/greenbids/real/time/data/v1/GreenbidsRealTimeDataProcessedAuctionRequestHook.java
Outdated
Show resolved
Hide resolved
...ks/modules/greenbids/real/time/data/v1/GreenbidsRealTimeDataProcessedAuctionRequestHook.java
Outdated
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Outdated
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Outdated
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Outdated
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Outdated
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Outdated
Show resolved
Hide resolved
...ks/modules/greenbids/real/time/data/v1/GreenbidsRealTimeDataProcessedAuctionRequestHook.java
Outdated
Show resolved
Hide resolved
...rg/prebid/server/hooks/modules/greenbids/real/time/data/core/GreenbidsInvocationService.java
Outdated
Show resolved
Hide resolved
| private boolean isImpKept(Imp imp, Map<String, Map<String, Boolean>> impsBiddersFilterMap) { | ||
| return impsBiddersFilterMap.get(imp.getId()).values().stream().anyMatch(isKept -> isKept); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that it is time for us to change the structure from Map<String, Map<String, Boolean>> to Map<String, Set<String>> impIdToKeptBidders (from the moment of creation in the processProbabilities method).
This is only a suggestion, it is not necessary to change it now.
| final List<Imp> updatedImps = updateImps(bidRequest, impsBiddersFilterMap); | ||
|
|
||
| final BidRequest updatedBidRequest = (isExploration || updatedImps.isEmpty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there is a potential bug with such implementation. You need to update bidRequest that provided in payloadUpdate(payload), so you don't miss updates from other modules in same group on same stage.
🔧 Type of changes
✨ What's the context?
We want to populate the
appliedTofield in the analytics tag and change the structure of analytics results to one imp per result. The example of the modified analytics tag results is as follows:🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
🔎 New Bid Adapter Checklist
🧪 Test plan
The changes are also reflected in the unit tests:
🏎 Quality check